Skip to content

Conversation

@nittoco
Copy link
Owner

@nittoco nittoco commented Oct 27, 2024

URL: https://leetcode.com/problems/construct-binary-tree-from-preorder-and-inorder-traversal/description/

Given two integer arrays preorder and inorder where preorder is the preorder traversal of a binary tree and inorder is the inorder traversal of the same tree, construct and return the binary tree.

@nittoco nittoco changed the title Construct Binary Tree from preorder and inorder traversal 105. Construct Binary Tree from preorder and inorder traversal Oct 27, 2024
if left_node_count:
root.left = self.buildTree(preorder[1:left_node_count + 1], inorder[:left_node_count])
if right_node_count:
root.right = self.buildTree(preorder[-right_node_count:], inorder[-right_node_count:])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ただの感想ですが、Python だと negative index で指定が出来るのですね。勉強になりました。
調べたら JavaScript でも、配列操作では使えないけど slice() メソッドでは使えるみたいですね (知らなかった): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/slice

Comment on lines +181 to +183
assert len(preorder) == len(inorder)
assert len(preorder) == len(set(preorder))
assert len(inorder) == len(set(inorder))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step 2 の中で、このような不正な値のチェックがあったりなかったりするのが気になりました。どういった意図でチェックする or しないようにしているのか知りたいです!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

すみません、最後の方は単純に書き忘れてただけです。
自分の方針としては全部書きたいなと思います

assert len(preorder) == len(set(preorder))
assert len(inorder) == len(set(inorder))
root = TreeNode(preorder[0])
stack = [(root, preorder, inorder)]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tuple だと各要素がどのような役割を持っているのか理解するのが少し大変なので、個人的には別のデータ構造を使ってほしい気持ちになりました。この場合そのままイミュータブルではあって欲しいので、NamedTuple や frozen 属性が付与されたdataclass あたりだと嬉しいです。特に後者は型ヒントも付与出来るので、実際のプロダクトで使われているとより安心感が強いですね。
あと、どのデータ構造を使うにしても、役割について一行程度のコメントが残っているとより親切に思います。

```python

class Solution:
def search_parent(self, child, root, node_val_to_inorder_index):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

開発チームの方針や好みにもよりますが、buildTree() には型ヒントが付与されているので、この場合はこちらにも付与してあげると良いのではないでしょうか。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

そうですね、型ヒントに慣れていないのでLeetCodeのデフォルトの分だけつけてましたが、あった方が親切ですね

right_subtree = SubtreeIndexRange(right_tree_size, right_preorder_min_index, right_inorder_min_index)
return left_subtree, right_subtree

def buildTree(self, preorder: List[int], inorder: List[int]) -> Optional[TreeNode]:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

必要以上に複雑な処理になっている印象を受けました。一度 inorder, preorder それぞれの配列の特徴と、最低限必要なデータが何かを整理して解法を考え直すと良いかもしれません。
手前味噌ですが、例えば私が同じ問題を解いたときの step 4 の最後の解答では、inorder の配列が頂点ノードが順に並んでいるという特徴から、これについては range を保持せず各イテレーションで一つずつカーソル (配列のポインタ) を進めるようにする等して、扱うデータを減らすように工夫しています: https://github.com/seal-azarashi/leetcode/pull/29/files

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントありがとうございます。すみません🙏、Step4の最終コードとコメントを見て理解できなかったのでまずは以下の疑問を解決したいのですが、
1, seal_azarashiさんのコードのCursorは何を指すのでしょうか。
2, inorder の配列が頂点ノードが順に並んでいるという特徴の「順」は何を差していますか

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nittoco
すいません返信が遅くなりました。

1, seal_azarashiさんのコードのCursorは何を指すのでしょうか。

preorder 配列のいずれかの要素を指すインデックスです。初期値が0で、buildTreeHelper() が呼ばれる度に値がインクリメント (一つ右に移動) されます。

2, inorder の配列が頂点ノードが順に並んでいるという特徴の「順」は何を差していますか

申し訳ありません、inorder と preorder を書き間違えていましたので訂正させてください。
「順」とは何かについてですが、これはツリーを preorder traversal した際の走査順序のことです。小さい画像で恐縮ですが、個人的に順序を示す図としてわかりやすかったものを添付しておきます:
Screenshot 2024-11-08 at 8 52 30

Copy link
Owner Author

@nittoco nittoco Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seal-azarashi
すみません🙏、理解に時間がかかり遅くなりました。図の方までありがとうございます

  • L356, L357の順に再帰すると、結果的にpreorderの順に探索することになるから、関数内で+1(L353)すればいいということですね。コードとしては簡潔になりますが、このように再帰するとpreorderになる、ということは読み手側に推理させる必要はありそうですね。(慣れた人ならすぐわかりそうなのかが微妙ですが、同様のロジックのここではわかりにくいというコメントもありますね)
  • 上の自分のコードについては自分の書き方がわかりにくかったですが、気持ちとしては末尾再帰の(「node.left = 再帰関数」のように再帰関数のreturnを受け取った後の処理をするのは、させない)コードを書く練習をしたい、というのがありました。なので処理する情報は多くなりそうですが、今見てみるとそれにしても改善の余地はありそうですね。考え直してみます。

```python
python
class Solution:
def is_inorder(self, node, next_node, inorder):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_inorder という関数名から、引数が inorder というものなのかどうかを判定する関数のように感じました。良いアイデアがないのですが、もう少し分かりやすい名前を付けたほうがよいと思います。

Comment on lines +24 to +26
node_searched = candidates.pop()
if not self.is_inorder(node_searched, child_node, inorder):
candidates.append(node_searched)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pop して append し直しているのは、動作として余計に感じます。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

そうですね、以前もsetで2回入れてる(結果的にsetだからOK)のが気になるというレビューをいただいたことがあり、冗長な操作をうっかりしてしまう癖がありそうなので気をつけます

Comment on lines +167 to +170
if direction == "left":
parent.left = node
else:
parent.right = node
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

これ、search_parent の中に入れてしまってもいい気がします。

Comment on lines +195 to +196
node_vals_preorder[1 : left_count + 1],
node_vals_inorder[:left_count],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

スライスがコピーなのは少し気になりますね。

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

自分もStackで配列を丸ごと持つのはちょっと気になりました。範囲のindexを持つ方がメモリ使用量的には良いですかね、とはいえ配列で操作する方が処理は簡単に書けそうなのと、stackには多くてもlog(n)個しか積まれないはずなので(あってる?)許容範囲内かなとは思います。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

そうですね、Index持つのだと実装が複雑なので、改善するならnumpyとか使ってviewにするかという感じですかね
https://discord.com/channels/1084280443945353267/1251052599294296114/1265343878072897557

parent_node.right = child_node
parent_candidates.append(child_node)
return root
```
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://discord.com/channels/1084280443945353267/1247673286503039020/1300957769477918791
私が前に考えていたのは、リンク先のようなものなのですが、たしかに、スタックの一つ上をみると、範囲が分かるので、範囲情報をスタックに足す必要はないかもしれないですね。

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

範囲情報をスタックから消してみた感想としては、結構非直感的になると思いました。

あるノードの left, right の下にぶら下げるかどうかの条件が、left の場合は、ぶら下げる先のノードの inorder における位置、right の場合は、ぶら下げる先のノードのスタック上の一つ上のノードの inorder における位置なのだけれども、これを分かりやすく説明するのが大変です。

速度の面では、こちらのほうの map や unordered_map のアクセスが増えて遅くなる要素が、あちらのほうの tuple が stack に載って遅くなる要素よりも、強く効くかしら。

class Solution {
public:
    TreeNode* buildTree(vector<int>& preorder, vector<int>& inorder) {
        map<int, int> inorder_position;
        for (int i = 0; i < inorder.size(); i++) {
            inorder_position[inorder[i]] = i;
        }
        // contains all nodes that may have a child.
        vector<TreeNode*> stack;
        TreeNode* root = new TreeNode(preorder[0]);
        stack.push_back(root);
        for (int i = 1; i < preorder.size(); i++) {
            TreeNode* node = new TreeNode(preorder[i]);
            int node_position = inorder_position[node->val];
            TreeNode* back = stack.back();
            if (node_position < inorder_position[back->val]) {
                back->left = node;
                stack.push_back(node);
                continue;
            }
            stack.pop_back();
            while (!stack.empty()) {
                TreeNode* parent = stack.back();
                if (node_position < inorder_position[parent->val]) {
                    break;
                }
                stack.pop_back();
                back = parent;
            }
            back->right = node;
            stack.push_back(node);
        }
        return root;
    }
};

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確かにleft_limit, right_limitの方が、やりたい操作に対して直接的な変数を定義してますね
操作の意図に合わせた変数の定義をするのも大事だなあと思いました。

def construct_left_tree(node):
child = None
while True:
parent = stack[-1] if stack else None
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここは、

if not stack:
    return child

分離したほうが好みです。

この関数意味としては「preorder にしたときに node よりも後ろに来るすべての stack の中のノードを .right で繋いで返す」ですね。そうすると、条件を満たさなくなるまでフィルターする動作と linked list をつなぐ動作の合わさったコードになりそうです。

Copy link
Owner Author

@nittoco nittoco Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

すみません、これは分離した方が好みというのは、その後の段落の文章が理由になってますか🙏(それぞれの文意はわかってるつもりですが繋がりが見えず)
後の段落の文章は関係なく、単に見やすいから分離しましょうという意図ならわかります。

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

「条件を満たさなくなるまでフィルターする動作」を書く時、こう書きたいなという気持ちがあるので、上の言い方になりましたが、確かに通じない言い方です。

while stack:
    parent = stack[-1]
    if condition:
        break
    move_to_next
return child

でもいいかもしれません。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほど、ありがとうございます。
自分は、stackの中身がなくなるまで(今まで見たnodeの中で、.rightで繋いとくべきものがもうない)というのも、条件を満たすものはこれ以上ないということで @oda さんのいうフィルター終了条件と捉えてましたが、わかりにくかったですかね。

node = TreeNode(val)
node.left = construct_left_tree(node)
stack.append(node)
dummy = stack[-1]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None を番兵にしてもいいんですが、私は construct_left_tree の引数を node_val_to_preorder_index[node.val] にすることで -inf を入れればすべて回収されるようにしました。
https://discord.com/channels/1084280443945353267/1247673286503039020/1300957861614063616

- 最初どういうことか分かってなかったが、自分のpositionと、stackに入っている自分の先祖の中で、自分より右にある中で一番左のpositionを入れると判定できるという案か
- 後者が、Step1の自分の実装では、結果的に一個上のnodeのpositionになってる
- この考えの方が自然なのか??
- odaさんの実装とは違い、右につながるやつの親はstackに入れっぱ
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

私の実装の気持ちは「stack に入っているものは、.right がまだ決まっていないもの」というつもりです。
https://discord.com/channels/1084280443945353267/1247673286503039020/1300957769477918791
.left がまだ決まっていないものは、別に変数をおいてもいいかもしれないけれども、stack[-1] なので、いいかなあと。

これは preorder 順に素直に構築していっていて、そのために必要な情報が何かを考えると出てきます。
途中まで構築していて preorder 順に作った新しいノードをどうするかを考えると
「一つ前につけたノード(.left 候補)」「ここまで作ってきて .right がまだ決まっていないノード(.right 候補)」のどれかにつけることになるはずです。preorder ということは一回でもそこはつけないと決まったらつきません。だからスタック管理で良くて、あとは、どこにつけるかを決めるのに最低限必要な情報をすべてのノードにもたせてやればいいです。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほど、「stack に入っているものは、.right がまだ決まっていないもの」「.left がまだ決まっていないものは、別に変数をおいてもいいかも」の辺で意図が結構分かってすっきりしました、ありがとうございます。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants